Issue #249 - Add strip to get() and getall() #260

felipeboffnunes · 2022-10-28T15:14:38Z

Resolves #249

codecov · 2022-10-28T15:16:43Z

Codecov Report

Merging #260 (7ce8789) into master (1913fb7) will not change coverage.
The diff coverage is n/a.

❗ Current head 7ce8789 differs from pull request most recent head b044b76. Consider uploading reports for the commit b044b76 to get more accurate results

@@            Coverage Diff            @@
##            master      #260   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          138       138           
  Branches        29        29           
=========================================
  Hits           138       138

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Gallaecio · 2022-10-28T15:18:01Z

parsel/selector.py

+    @typing.overload
+    def get(self, strip: bool) -> str:
+        pass


Why is this necessary? (I am not saying it is not)

I don't think it is, I just saw the overloads above this line and thought it was required

@typing.overload def get(self, default: None = None) -> Optional[str]: pass @typing.overload def get(self, default: str) -> str: pass

I removed it on the last push. I don't understand the whole overload completely, but I feel those 2 are also unnecessary?

I think it is related to typing, when parameter types affect output type, but I do not think it applies here.

Yes, removing it broke the pipeline for type checks.

No overload variant of "get" of "SelectorList" matches argument type "bool"

As you added a new argument to the method you need to add them to all overloads too.

And those two overloads are needed because the method either can return None if default wasn't passed or it returns the default instead, so this describes the typing info more clearly.

tests/test_selector.py

…type "bool"

Gallaecio · 2022-10-28T15:28:38Z

parsel/selector.py

@@ -200,12 +200,15 @@ def re_first(
            return el
        return default

-    def getall(self) -> List[str]:
+    def getall(self, strip: bool = False) -> List[str]:


Let’s make it keyword-only.

Suggested change

def getall(self, strip: bool = False) -> List[str]:

def getall(self, *, strip: bool = False) -> List[str]:

Same on get.

Should we apply this to typing.overload of get as well?

I tried adding it to get but then this would require some type check on default entry to validate it is a str, either way the behavior looks fine without it for get

wRAR · 2022-10-28T15:40:47Z

parsel/selector.py

+    @typing.overload
+    def get(self, strip: bool) -> str:
+        pass


As you added a new argument to the method you need to add them to all overloads too.

wRAR · 2022-10-28T15:41:54Z

parsel/selector.py

+    @typing.overload
+    def get(self, strip: bool) -> str:
+        pass


And those two overloads are needed because the method either can return None if default wasn't passed or it returns the default instead, so this describes the typing info more clearly.

wRAR · 2022-10-28T15:43:59Z

parsel/selector.py

        """
        Call the ``.get()`` method for each element is this list and return
        their results flattened, as a list of strings.
        """
-        return [x.get() for x in self]
+        data = [x.get() for x in self]


I think you can just pass strip to get() instead of essentially reimplementing the new code from get() here?
(granted, the code is almost trivial, but still)

x is a Selector, at that point neither default nor strip exist anymore
Only SelectorList recognizes those two fields

About adding the strip argument to the other overloads, wouldn't that remove the purpose of the overload on those?
Wouldn't having an overloaded default + strip be just going back to the non-overloaded method signature?

x is a Selector, at that point neither default nor strip exist anymore

Aren’t you also adding strip to Selector? Can’t you use data = [x.get(strip=True) for x in self]?

That ended up looking harder than it should when I tried.
Also as far as I could see, default also is not on the Selector, so for standardization, I believe adding it would also be required. And in that case I don't understand why it wasn't added up until now, it gives me the feeling it is not a trivial task given the current implementation of get on Selector.

Yeah, you are changing Selector.get() to have strip and it already has default so I think there shouldn't be any problems with using it, and it indeed doesn't look too complicated to me :)

Also if you have issues with typing I can hep, I'm working on improving parsel typing right now.

this_doesnt_work = sel.xpath("//ul/li[position()>1]")[0].get(default="6") # Selector doesnt have default field

sel.xpath(...).get(default=...) works in normal parsel without your changes.

About adding the strip argument to the other overloads, wouldn't that remove the purpose of the overload on those?

No, the purpose of those overloads is to show that the type of the return value depends on whether default is None or not.

Wouldn't having an overloaded default + strip be just going back to the non-overloaded method signature?

I believe just adding strip to the existing overloads is enough, no new overloads are needed.

sel.xpath(...).get(default=...) uses SelectorList, not Selector. The problem is that by indexing [0] then you go to a Selector, and it does not have default .
but either way I still have to give it a look

Also, it seems a little unintuitive to be using the [0] when doing a get operation, as you are already asking the SelectorList to get the first instance it found. I used it on my tests because I saw some of the tests use this kind of indexing to prove Selector behaves correctly, and in this case, I believe you should be able to do both default and strip directly on a Selector, but you can't for a Selector at the moment so maybe this needed further discussion.

Wait, you are right. Selector.get() is much simpler than SelectorList.get(). Well, I think it makes sense and is easy to add strip= to Selector.get().

it seems a little unintuitive to be using the [0] when doing a get operation, as you are already asking the SelectorList to get the first instance it found

Yeah, it's probably unusual to write <SelectorList>[0].get(). It's OK to have a Selector and call a get) on it though.

I believe you should be able to do both default and strip directly on a Selector, but you can't for a Selector at the moment so maybe this needed further discussion.

Not so sure about default=, but maybe there are use cases where it's useful.

I think we can keep this as is, strip for Selector.get() is less useful and can be implemented separately.

parsel/selector.py

kmike · 2022-11-01T06:54:17Z

parsel/selector.py

@@ -200,30 +200,36 @@ def re_first(
            return el
        return default

-    def getall(self) -> List[str]:
+    def getall(self, *, strip: bool = False) -> List[str]:
        """
        Call the ``.get()`` method for each element is this list and return
        their results flattened, as a list of strings.


I think the new option needs to be mentioned in the docstring.

kmike · 2022-11-01T06:54:38Z

parsel/selector.py

-    def get(self, default: Optional[str] = None) -> Optional[str]:
+    def get(
+        self, default: Optional[str] = None, strip: bool = False
+    ) -> Optional[str]:
        """
        Return the result of ``.get()`` for the first element in this list.
        If the list is empty, return the default value.


The new option needs to be mentioned in the docstring

kmike · 2022-11-01T06:55:53Z

Hey! Is there a use case for this feature if #127 is merged, do you have some examples?

I'm asking because sel.get(text=True) would also be stripping text by default, so strip=True won't be useful with text argument. It may be also hard to support strip=False, text=True. And I wonder if there are real-world cases where you want strip=True, but don't want text=True.

felipeboffnunes · 2022-11-01T10:53:18Z

@kmike By the time I saw #127 I had most of the bulk of the strip logic coded. I do0n't think there is a common use case for this PR compared to 127 besides maybe being a more straight forward pre processing step of the text where only one thing happens (strip).
Either way I felt that #127 has been around for a good while and not finished, so a simpler alternative was presented just to fix the issue #249
I guess we can close this one, mention #127 in #249 and try to close the features presented there

Issue scrapy#249 - Add strip to get() and getall()

fbdb881

Gallaecio reviewed Oct 28, 2022

View reviewed changes

tests/test_selector.py Outdated Show resolved Hide resolved

Gallaecio approved these changes Oct 28, 2022

View reviewed changes

Felipe Boff Nunes added 2 commits October 28, 2022 12:23

cleanup

cbc6e89

Fix: No overload variant of "get" of "SelectorList" matches argument …

02b09b0

…type "bool"

Gallaecio reviewed Oct 28, 2022

View reviewed changes

Keyword-only strip for getall.

4185e19

wRAR reviewed Oct 28, 2022

View reviewed changes

wRAR reviewed Oct 31, 2022

View reviewed changes

parsel/selector.py Outdated Show resolved Hide resolved

Felipe Boff Nunes added 6 commits October 31, 2022 12:09

Remove Optional from get strip argument

ff23fdb

Develop, commit, fail black, adjust, repeat

37dfd36

Adjust overload get

cbe7042

Adjust overload get

43ba89f

Attempt strip only overload on typing check

7ce8789

This seems to fix typing (?)

b044b76

kmike reviewed Nov 1, 2022

View reviewed changes

bblanchon mentioned this pull request Aug 11, 2023

Adding a strip kwarg to get() and getall() #249

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #249 - Add strip to get() and getall() #260

Issue #249 - Add strip to get() and getall() #260

felipeboffnunes commented Oct 28, 2022 •

edited by Gallaecio

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading

Gallaecio Oct 28, 2022

felipeboffnunes Oct 28, 2022

Gallaecio Oct 28, 2022

felipeboffnunes Oct 28, 2022

wRAR Oct 28, 2022

wRAR Oct 28, 2022

Gallaecio Oct 28, 2022

Gallaecio Oct 28, 2022

felipeboffnunes Oct 28, 2022

felipeboffnunes Oct 28, 2022 •

edited

Loading

wRAR Oct 28, 2022

wRAR Oct 28, 2022

wRAR Oct 28, 2022

felipeboffnunes Oct 28, 2022

felipeboffnunes Oct 28, 2022

Gallaecio Oct 29, 2022

felipeboffnunes Oct 29, 2022

wRAR Oct 29, 2022

wRAR Oct 29, 2022

felipeboffnunes Oct 30, 2022 •

edited

Loading

wRAR Oct 30, 2022

wRAR Oct 31, 2022

kmike Nov 1, 2022

kmike Nov 1, 2022

kmike commented Nov 1, 2022 •

edited

Loading

felipeboffnunes commented Nov 1, 2022

	def getall(self, strip: bool = False) -> List[str]:
	def getall(self, *, strip: bool = False) -> List[str]:

Issue #249 - Add strip to get() and getall() #260

Are you sure you want to change the base?

Issue #249 - Add strip to get() and getall() #260

Conversation

felipeboffnunes commented Oct 28, 2022 • edited by Gallaecio Loading

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeboffnunes Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeboffnunes Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmike commented Nov 1, 2022 • edited Loading

felipeboffnunes commented Nov 1, 2022

felipeboffnunes commented Oct 28, 2022 •

edited by Gallaecio

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading

felipeboffnunes Oct 28, 2022 •

edited

Loading

felipeboffnunes Oct 30, 2022 •

edited

Loading

kmike commented Nov 1, 2022 •

edited

Loading